Code Review Summary

Thanks for this PR! The page-based read cache is a well-structured addition. Here are my findings:

Already Noted (by self-review)

• Page constructor null check — Allocate return value not checked (page.h)
• GetOrLoadPage mutex contention — cache mutex held during disk I/O (basic_io.h)
• PageCache::Insert eviction logic — fallback to pages_.begin()->first() (page_cache.cpp)

Additional Findings

[suggestion] GetMemoryUsage doesn't account for cache memory
The BasicIO::GetMemoryUsage() method only returns size_, not including the page cache memory. When the read cache is enabled, the actual memory footprint can be significantly larger, leading to inaccurate memory budgeting. Consider adding cache_->Size() * Page::DEFAULT_PAGE_SIZE to the returned value.

[suggestion] Serialize populates cache with transient reads
Serialize calls this->Read(...) which goes through ReadCached when the cache is enabled. This populates the cache with every page read during serialization, potentially evicting useful pages that were already cached. Consider clearing the cache before serialization or adding a bypass flag.

[note] Shrink return removal
The Shrink method previously had return cast().ShrinkImpl(size); which prevented the else branch from executing. Now the return is removed so ClearCache() always executes. This is correct behavior, but worth confirming no ShrinkImpl implementation relied on the old early-return.

[suggestion] DirectReadImpl always sets need_release=true with cache
When the cache is active, the DirectReadImpl overload that returns const uint8_t* always allocates and copies, setting need_release = true. This changes semantics for callers that previously relied on zero-copy reads. Worth documenting.

[note] BucketDataCell GetMemoryUsage under-reports
AdjustBucketReadCacheParam creates separate IO instances per bucket, each with its own cache, but BucketDataCell::GetMemoryUsage() doesn't include cache memory. Consistent with existing IOArray behavior, but worth noting for capacity planning.

Overall Assessment

The design is clean: pluggable eviction policy (PageCache base + LRUPageCache), shared_ptr ownership so evicted pages remain valid for in-flight readers, and proper cache invalidation on writes. The test coverage is good with unit tests for PageCache/LRUPageCache and integration tests for the full read/write/invalidation flow. The examples are helpful for demonstrating usage.

No blocking issues found. The suggestions above are non-blocking improvements.

Automated inline review completed.

Review effort: high (1407 changed lines across 39 files).
Submitted 2 inline comments.

Reviewed commit 44c70ef.

Automated inline review completed.

Review effort: high (1420 changed lines across 39 files).
Submitted 4 inline comments.

Reviewed commit a5f9bb5.

Code review for PR #2567: Add optional read cache for disk IO.

Summary: This PR introduces a page-based read cache (128KB pages) with LRU eviction for disk IO backends. The cache integrates into the CRTP-based BasicIO<IOTmpl> template and is enabled via IO parameters. IVF bucket datacells distribute the cache budget evenly across buckets.

Overall Assessment: The design is clean and well-integrated into the existing IO abstraction. The cache correctly handles write-through invalidation, resize/shrink/deserialize cache clearing, and cross-page boundary reads. The test coverage is reasonable for unit-level testing of the cache components.

General Notes:

1. [note] DirectReadImpl bypass with cache: When the read cache is enabled, DirectReadImpl is bypassed entirely — the cache path allocates a new buffer, copies data into it via ReadCached, and the caller must call Release to free it. This changes the semantics from zero-copy (returning an internal pointer) to always-copy when the cache is active. Callers that rely on need_release == false for performance (e.g., scan_bucket_by_id in bucket_datacell.h) will now always pay the allocation + copy cost. This is expected given the cache architecture, but worth being aware of for hot paths.

2. [suggestion] GetMemoryUsage does not include cache: The GetMemoryUsage() method only returns size_, which does not account for the memory consumed by the page cache. When the read cache is enabled, the actual memory footprint can be significantly larger, leading to inaccurate memory budgeting. Consider including cache memory: usage += cache_->Size() * Page::DEFAULT_PAGE_SIZE.

3. [suggestion] DirectReadImpl overload documentation: The DirectReadImpl overload (returning const uint8_t* with need_release) allocates a temporary buffer and copies through ReadCached. When the cache is active, this always sets need_release = true. It would be helpful to document this behavioral change in the method comment.

4. [note] NonContinuousIO now routes through BasicIO: The change from inner_io_->WriteImpl(...) to inner_io_->Write(...) (and similarly MultiReadImpl → MultiRead) correctly routes through BasicIO's public interface which handles cache invalidation and cache reads. This is a necessary change for read cache correctness.

5. [note] Shrink now always clears cache: The Shrink method previously had return cast().ShrinkImpl(size); which prevented the else branch from executing. Now ClearCache() always executes after Shrink. This is correct behavior, but worth confirming no ShrinkImpl relies on the old early-return behavior.

See inline comments for specific findings.

Automated inline review completed.

Review effort: high (1460 changed lines across 40 files).
Submitted 3 inline comments.

Reviewed commit 5b2e73d.

Automated inline review completed.

Review effort: high (1500 changed lines across 41 files).
Submitted 1 inline comment.

Reviewed commit 03199f7.

Code review for commit 4d926ac.

Automated inline review completed.

Review effort: high (1501 changed lines across 41 files).
Submitted 3 inline comments.

Reviewed commit 4d926ac.

lgtm

[P2] RebuildSupplementIOParam does not halve cache in hybrid mode

In src/datacell/rabitq_split_datacell.h, RebuildSupplementIOParam (line 1002-1015) builds a custom JSON parameter when supplement_io_type_ is non-empty (hybrid IO mode), but never applies the split_cache halving logic. This means in hybrid mode both the one-bit and supplement cells each get the full cache size — 2× the expected memory budget.

The constructor path (line 186) correctly passes split_cache=true for the supplement cell, but RebuildSupplementIOParam (used during InitIO) only appends the suffix path and IO type override. The read_cache_total_size_ from the original io_param flows through unchanged.

Suggested fix: add the same split_cache logic inside the !supplement_io_type_.empty() branch:

if (io_param->enable_read_cache_) {
    json[READ_CACHE_TOTAL_CACHE_SIZE_KEY].SetUint64(io_param->read_cache_total_size_ / 2);
}

Automated inline review completed.

Review effort: high (1501 changed lines across 41 files).
Submitted 3 inline comments.

Reviewed commit 937ad55.